Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some adaptations to workflows #9994

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yngve-sk
Copy link
Contributor

@yngve-sk yngve-sk commented Feb 6, 2025

Issue
Resolves equinor/semeio#685

Add support for defaulted kwargs from workflows

@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 2 times, most recently from e38fc26 to 16d8860 Compare February 6, 2025 11:11
Copy link

codspeed-hq bot commented Feb 6, 2025

CodSpeed Performance Report

Merging #9994 will not alter performance

Comparing yngve-sk:25.02.05.internal-workflows-pass-some-stuff (1e668d0) with main (cc15585)

Summary

✅ 25 untouched benchmarks

@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 3 times, most recently from a1fc9a1 to ab916da Compare February 6, 2025 13:40
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I have some minor questions.

runner = WorkflowRunner(
workflow=workflow,
storage=storage,
ert_config=ert_config,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should remove this as it is not available in run models, where most of these are run from?

if is_using_wf_args_fixture
else [*workflow_args, *fixture_args]
)
if not positional_args and not use_kwargs:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we could keep this as it used to be? I dont think kwargs were really supported before, they just ended up being workflow_args in a list?

@@ -107,14 +107,16 @@ class WorkflowRunner:
def __init__(
self,
workflow: Workflow,
config_file: str | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also commented in semeio, could potentially give reports dir, we do something similar for the update report, that has a folder it writes to.

storage: Storage | None = None,
ensemble: Ensemble | None = None,
ert_config: ErtConfig | None = None,
) -> None:
self.__workflow = workflow
self.storage = storage
self.ensemble = ensemble
self.ert_config = ert_config
self.ert_config = ert_config # Should eventually be removed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove now?

@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 8 times, most recently from a60c42c to 6e47da3 Compare February 11, 2025 08:55
@jonathan-eq jonathan-eq self-requested a review February 11, 2025 09:13
@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 3 times, most recently from 87c7ada to 0aeee68 Compare February 11, 2025 13:11
Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good :) See some minor questions 🔍

src/ert/config/workflow_fixtures.py Outdated Show resolved Hide resolved
src/ert/gui/tools/plugins/plugin.py Outdated Show resolved Hide resolved
src/ert/workflow_runner.py Outdated Show resolved Hide resolved
tests/ert/unit_tests/config/test_ert_plugin.py Outdated Show resolved Hide resolved
tests/ert/unit_tests/config/test_ert_plugin.py Outdated Show resolved Hide resolved
@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 0aeee68 to 9d1a66e Compare February 11, 2025 14:20
@jonathan-eq
Copy link
Contributor

There seems to be a GUI test hanging now 🤔

@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from b828aa7 to f094ca8 Compare February 12, 2025 11:07
@yngve-sk yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from f094ca8 to 1e668d0 Compare February 12, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AHMAnalysis not working after ErtConfig change
3 participants